feat: Update Chain Reorg Detection and Handling #187
Conversation
…ore emitting live
This reverts commit 9a63182.
This reverts commit 8169ed7.
| let sync_handler = SyncHandler::new( | ||
| self.provider.clone(), | ||
| self.max_block_range, | ||
| start_id, | ||
| block_confirmations, | ||
| sender, | ||
| ); | ||
| sync_handler.run().await |
There was a problem hiding this comment.
this is a strategy-like pattern that we could use for mode handling. The logic is all contained within the strategy type.
…catchup to live before emitting
|
Depends on #205 , but can be reviewed independently even now EDIT: #205 will solve test failures such as https://github.com/OpenZeppelin/Event-Scanner/actions/runs/19633672074/job/56219119078?pr=187, it's an issue with the test setup |
LeoPatOZ
left a comment
LeoPatOZ
left a comment
There was a problem hiding this comment.
Leaving these here for now, these are mostly very very initials thoughts just as i was reading through the PR (hence why they seem to be so disjunct).
Next couples of passes should hopefully be more focused on individual flows
| block_confirmations: u64, | ||
| max_block_range: u64, | ||
| reorg_handler: &mut ReorgHandler<N>, | ||
| notify_after_first_block: bool, |
There was a problem hiding this comment.
nit: I feel like a lot of the these common function have similar arguments. Perhaps it would be nicer to define a stream config struct or similar, and pass that in?
pub(crate) async fn stream_live_blocks<N: Network>(
stream_config: StreamConfig,
notify_after_first_block: bool,
)hopefully can be reduced to something like this? (didnt check which params are shared between the fns)
There was a problem hiding this comment.
Maybe can be shared with sync_handler.rs as well (seems to share similar fields)
There was a problem hiding this comment.
Maybe we extracted these functions out of the block range scanner too preemptively?
I feel like they need so many of the same arguments that the block range scanner contains that it may as well be an impl of the connected scanner itself? Maybe in that case we have the reorg handler be a field of the block range scanner?
Im not exactly sure what the cleanest way to approach this is :(
We can also think about separating the block range scanner by type just like the event scanner. We could have LiveBlockRangeScanner SyncFromBlockRange etc.. Perhaps it would make our lives easier
Apologies Im just thinking out loud so im putting my thoughts here for now
There was a problem hiding this comment.
Everything you wrote makes complete sense, and were it not for the fact that with parallelization the whole of block range scanner will probably become redundant, I would've applied every suggestion.
Currently it makes no sense to invest more time into refactoring any of its components.
| &mut self, | ||
| block: &N::BlockResponse, | ||
| ) -> Result<Option<N::BlockResponse>, ScannerError> { | ||
| let block = block.header(); |
There was a problem hiding this comment.
Was thinking about #102
Perhaps this check could be skipped if block num < finalised ?
There was a problem hiding this comment.
We'd need to eagerly fetch finalized block (1 RPC call) and if block num > finalised, we'd need to perform another RPC call to perform the current check (2 RPC calls in total).
Currently, we only ever perform 1 RPC call.
| batch_count += 1; | ||
| if batch_count % 10 == 0 { | ||
| debug!(batch_count = batch_count, "Processed historical batches"); | ||
| } |
There was a problem hiding this comment.
nit: do you mind if we remove this not sure its that helpful and makes the fn harder to read imo
Maybe this info can be emitted in a different log
There was a problem hiding this comment.
I don't want to intentionally introduce unrelated changes to the PR. If you have an idea where this could be emitted instead, feel free to open a small PR
Resolves #56
Resolves #76
Resolves #5
Resolves #155